cmd/list, pkg/podman: Eliminate getContainers() by doing everything in podman.GetContainers()#1775
Conversation
The getContainers() function originated as listContainers(), when it invoked 'podman ps --filter label=...' through podman.GetContainers() separately for each label used to identify Toolbx containers. The label was specified using the 'args' parameter. This changed in commit 2369da5 when getContainers() started invoking 'podman ps' only once through podman.GetContainers() to get all available containers, with the 'args' parameter set to a fixed value. containers#1775
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the way arguments are passed to the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
debarshiray
force-pushed
the
wip/rishi/cmd-list-pkg-podman-getContainers
branch
from
01de68a to
ef0b302
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors how containers are fetched by moving the podman ps arguments from cmd/list.go into pkg/podman/podman.go. This is a good simplification. My review includes a suggestion to take this refactoring a step further by also moving the container filtering logic into the podman package, which would better align with the PR's title and improve code modularity.
... not getContainers(). This is a step towards eliminating the code in getContainers() by doing everything in podman.GetContainers(). This removes one unnecessary layer of code. The getContainers() function originated as listContainers(), when it invoked 'podman ps --filter label=...' through podman.GetContainers() separately for each label used to identify Toolbx containers, and then joined and sorted the results. This changed in commit 2369da5 when getContainers() started invoking 'podman ps' only once through podman.GetContainers() to get all available containers, and then filtered them itself based on the labels. Before this change in commit 2369da5, it probably made some sense to keep podman.GetContainers() only as a thin wrapper to invoke 'podman ps' with different options, and to do everything else in getContainers(). However, since then more is being done inside the podman package (eg., unmarshalling the 'podman ps' JSON in commit d032302), and getContainers() is the only caller of podman.GetContainers(). Therefore, it looks awkward to have the code to get all Toolbx containers split across two functions in different packages. containers#1775
|
Build succeeded. ✔️ unit-test SUCCESS in 3m 12s |
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 25s |
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 22s |
1 participant
These are steps towards eliminating the code in
getContainers()by doing everything inpodman.GetContainers().The
getContainers()function originated aslistContainers(), when it invokedpodman ps --filter label=...throughpodman.GetContainers()separately for each label used to identify Toolbx containers, and then joined and sorted the results. This changed in commit 2369da5 whengetContainers()started invokingpodman psonly once throughpodman.GetContainers()to get all available containers, and then filtered them itself based on the labels.Before this change in commit 2369da5, it probably made some sense to keep
podman.GetContainers()only as a thin wrapper to invokepodman pswith different options, and to do everything else ingetContainers(). However, since then more is being done inside thepodmanpackage (eg., unmarshalling thepodman psJSON in commit d032302), andgetContainers()is the only caller ofpodman.GetContainers().Therefore, it looks awkward to have the code to get all Toolbx containers split across two functions in different packages.